Promotions: Deprecate Spree::PromotionRule#actionable and split rules that implement it #4321

mamhoff · 2022-03-30T15:29:31Z

The API of promotion rules is a bit strange: There's the applicable method, which looks like promotion rules could apply to line items as well as orders, and there's actionable?, which is like eligible?, but for line items.

The Product rule is really two rules in one: An order rule (Does this order contain product x?) and a line item rule (Is this line item product x?).

Same with the Taxon rule:

Does the order contain a product with taxon x?
Does the line item have a product with taxon x?

Same with the OptionValue rule:

Does the order contain a variant with option value X?
Does the line item have a variant with option value X?

What this PR does is

split those rules up into two
migrate people's promotions that use these rules such that they now have the additional "line item" rule as well, preserving behavior in existing stores
deprecate Spree::Promotion#line_item_actionable?
deprecate Spree::PromotionRule#actionable?

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I've changed the guides to reflect my changes

jarednorman

This looks good, though we could additionally deprecate the old rule if we want, right?

mamhoff · 2022-03-31T15:44:22Z

I've found a bug in the promotion system that makes line-item level rules do things we don't want them to do, so I'm marking this as draft until I've got the fix for that into a state that works. I'm likely also going to split the other two rules that have actionable? and deprecate that and related methods. There is a little bit of UI work left that should help admins understand the new system a lot better, as well.

In this PR, I'm moving the Product rule to an OrderProduct rule, but given that this is an STI class, and deprecating classes requires changes to the type column, I think I'm rather going to build an upgrade path that simply adds the line items rules to any promotions that have rules with actionable?.

mamhoff · 2022-04-01T11:55:24Z

Hm, this got a bit bigger, but I think I got to a good place with this. I'm not deprecating the old rules; instead I leave them to do what their names and documentation and implementation say that they do: Check order eligibility. I added new rules for line item eligibility, and deprecated the use of actionable? for line items.

This change gives meaning to the applicable? method, and it allows to extend the promotion system to also support checks for e.g. shipments or shipping rates in the future.

mamhoff · 2022-04-01T11:56:02Z

@jarednorman I've re-requested your review because this now contains a bunch more change than what you initially reviewed.

tvdeyen

Very nice work. Thank you for removing some of the confusing parts of the promotion system.

core/app/models/spree/promotion/rules/line_item_product.rb

core/config/locales/en.yml

backend/spec/features/admin/promotions/product_rule_spec.rb

core/app/models/spree/promotion/rules/line_item_taxon.rb

core/app/models/spree/promotion/rules/option_value.rb

core/app/models/spree/promotion.rb

This rule is like "Product", but only applies to line items.

This rule is now only applicable to an order, and the naming should reflect that.

This spec allows me to show some of the weird things in the promotion system.

When we have a promotion with a line item rule that evaluates to ineligible for a line item, it will still call "actionable?" on that rule, which will evaluate to `true`, and create an adjustment on line items that will always be ineligible.

There was only one option for the match policy, so we can get rid of it.

This promotion rule shall replace the use of `actionable` on the `OptionValue` promotion rule.

This Rule can replace the use of `actionable?` on the taxon promotion rule.

Now that the Order, Taxon, and OptionValue rules only operate on orders, we need to add the respective line item rules to the promotions that need them.

This can now be accomplished using the LineItemActionable rule.

This can be done with the LineItemTaxon rule now.

This method now runs through promotion.eligible, which checks for the presence of actions.

We now have a few more promotion rules.

tvdeyen

Amazing

mamhoff · 2022-05-09T11:23:14Z

@kennyadsl would you mind having a look? Would this also be a change for 4.0?

kennyadsl · 2022-06-07T11:09:24Z

@mamhoff nope, if the public methods are deprecated and there's a clear transition path for who needs to change something in their codebase, I think we can marge this before 4.0 and only remove the deprecated parts in that major.

Still, I didn't have time to review this PR yet, it's a bit large and requires some effort. Do you think there's a way or makes sense to split this into smaller PRs?

mamhoff · 2022-06-07T11:43:42Z

@kennyadsl I don't think I can meaningfully split it into smaller pull requests. I'm depecrating an API here, and I'm providing a replacement. I'm also implementing this change across the promotion rules we have. This will necessarily take a few LOC, but splitting it up over multiple PRs I think would hurt readability and understandability of what's going on. Sorry! But do take your time. Best!

kennyadsl · 2022-06-10T13:35:39Z

Update: I had a quick look and this looks very good. I also asked some team members to check if this would impact the stores they are working on. I will approve once I have positive answers. In the meantime, thanks for the fantastic work, Martin!

mamhoff · 2022-07-20T11:56:38Z

I think I do need to add more stuff from #4296 to this PR. Notably this will introduce bugs when re-calculating adjustments.

mamhoff · 2023-11-03T11:27:13Z

Closing in favor of https://github.com/friendlycart/solidus_friendly_promotions/

jarednorman approved these changes Mar 30, 2022

View reviewed changes

mamhoff marked this pull request as draft March 31, 2022 15:41

mamhoff force-pushed the line-item-product-rule branch 3 times, most recently from b3d19a8 to 025581a Compare April 1, 2022 11:38

mamhoff marked this pull request as ready for review April 1, 2022 11:53

mamhoff requested a review from jarednorman April 1, 2022 11:55

mamhoff changed the title ~~Promotions: Split Product Rule into Two~~ Promotions: Deprecate Spree::PromotionRule#actionable and split rules that implement it Apr 1, 2022

mamhoff force-pushed the line-item-product-rule branch from c2dd392 to b17739d Compare April 1, 2022 12:01

mamhoff mentioned this pull request Apr 4, 2022

Discount system #4296

Closed

5 tasks

tvdeyen reviewed Apr 11, 2022

View reviewed changes

mamhoff added 16 commits April 19, 2022 11:27

Add LineItemProduct Rule

a2ee657

This rule is like "Product", but only applies to line items.

Rename Product rule

1751334

This rule is now only applicable to an order, and the naming should reflect that.

Fix promotion adjustments spec to not use Product rule

6479f41

Add promotion integration spec

46bbd9d

This spec allows me to show some of the weird things in the promotion system.

Prevent ineligible line item adjustments

9faa855

When we have a promotion with a line item rule that evaluates to ineligible for a line item, it will still call "actionable?" on that rule, which will evaluate to `true`, and create an adjustment on line items that will always be ineligible.

Simplify Promotion Rule Option Value

8c68e2b

There was only one option for the match policy, so we can get rid of it.

Add Promotion Rule LineItemOptionValue

ecf07c1

This promotion rule shall replace the use of `actionable` on the `OptionValue` promotion rule.

Add Promotion Rule "LineItemTaxon"

ab5b9bd

This Rule can replace the use of `actionable?` on the taxon promotion rule.

Add migration to add necessary line item rules to existing promotions

bf2b180

Now that the Order, Taxon, and OptionValue rules only operate on orders, we need to add the respective line item rules to the promotions that need them.

Remove "actionable?" from Product rule

4ad9f82

Option Value rule: Remove #actionable?

631ed8a

This can now be accomplished using the LineItemActionable rule.

Remove Rules::Taxon#actionable?

5b7fc5a

This can be done with the LineItemTaxon rule now.

Deprecate Promotion#line_item_actionable and PromotionRule#actionable?

bc5f2b2

Fix DistributedAmounts spec

94bed4e

Fix CreateItemAdjustments spec

eb70716

Fix for Promotion#line_item_eligible spec

960fde9

This method now runs through promotion.eligible, which checks for the presence of actions.

User Guides: Update Promotion Rules page

345d0f3

We now have a few more promotion rules.

mamhoff force-pushed the line-item-product-rule branch from b17739d to 2250f78 Compare April 19, 2022 09:35

Update Developer Guides for new Promotion API

2f5e451

mamhoff force-pushed the line-item-product-rule branch from 2250f78 to 2f5e451 Compare April 19, 2022 12:26

tvdeyen approved these changes Apr 20, 2022

View reviewed changes

mamhoff marked this pull request as draft July 20, 2022 11:56

kennyadsl added the type:enhancement Proposed or newly added feature label Aug 23, 2022

waiting-for-dev added changelog:solidus_core Changes to the solidus_core gem changelog:solidus_backend Changes to the solidus_backend gem labels Aug 30, 2022

mamhoff closed this Nov 3, 2023

mamhoff deleted the line-item-product-rule branch November 3, 2023 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promotions: Deprecate Spree::PromotionRule#actionable and split rules that implement it #4321

Promotions: Deprecate Spree::PromotionRule#actionable and split rules that implement it #4321

mamhoff commented Mar 30, 2022 •

edited by kennyadsl

Loading

jarednorman left a comment

mamhoff commented Mar 31, 2022

mamhoff commented Apr 1, 2022

mamhoff commented Apr 1, 2022

tvdeyen left a comment

tvdeyen left a comment

mamhoff commented May 9, 2022

kennyadsl commented Jun 7, 2022 •

edited

Loading

mamhoff commented Jun 7, 2022

kennyadsl commented Jun 10, 2022

mamhoff commented Jul 20, 2022

mamhoff commented Nov 3, 2023

Promotions: Deprecate Spree::PromotionRule#actionable and split rules that implement it #4321

Promotions: Deprecate Spree::PromotionRule#actionable and split rules that implement it #4321

Conversation

mamhoff commented Mar 30, 2022 • edited by kennyadsl Loading

jarednorman left a comment

Choose a reason for hiding this comment

mamhoff commented Mar 31, 2022

mamhoff commented Apr 1, 2022

mamhoff commented Apr 1, 2022

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

mamhoff commented May 9, 2022

kennyadsl commented Jun 7, 2022 • edited Loading

mamhoff commented Jun 7, 2022

kennyadsl commented Jun 10, 2022

mamhoff commented Jul 20, 2022

mamhoff commented Nov 3, 2023

mamhoff commented Mar 30, 2022 •

edited by kennyadsl

Loading

kennyadsl commented Jun 7, 2022 •

edited

Loading